lib/undo_manager.ex (4)

89-91: Minor documentation formatting issue.

The code example in the documentation is missing its closing comment. Add a closing comment to maintain consistency.

      Yex.Text.insert(text, 6, "Second ")
-      # Now these will be separate undo operations
+      # Now these will be separate undo operations
+      ```

---

115-120: Improve documentation clarity.

The documentation explains the event ID addition but could be more concise and better formatted. Consider restructuring this section.

-    WE ADD AN ID TO meta as event.meta.event_id
-    Event.id is added in undo.rs for a specific reason: we need a unique id to track each stack item and no unique info is provided in the Event.
-    So when we get an event from Rust, we add an id to it.
-    This is used by the metadata GenServer to track the metadata for each stack item.
-    Why not use yrs UndoManager to track the metadata?  Because we cannot update the mutable metadata in a NIF callback context without potentially blocking.
+    The Event struct is extended with a unique event_id in the meta field for the following reasons:
+    - Enables tracking of individual stack items
+    - Facilitates metadata management through the GenServer
+    - Avoids potential blocking in NIF callback contexts that would occur if using yrs UndoManager

---

243-252: Enhance error handling consistency across observer callbacks.

The observer callbacks (on_item_added, on_item_updated, on_item_popped) handle errors similarly but could benefit from more detailed error information.

Consider adding specific error tuples for each failure case:

   def on_item_added(%__MODULE__{} = manager, callback) when is_function(callback, 1) do
     with {:ok, manager} <- ensure_metadata_server(manager),
          :ok <-
            Yex.UndoMetadataServer.set_item_added_callback(manager.metadata_server_pid, callback),
          :ok <-
            Yex.Nif.undo_manager_observe_item_added(manager.reference, manager.metadata_server_pid) do
       {:ok, manager}
+    else
+      {:error, reason} -> {:error, {:observer_setup_failed, reason}}
     end
   end

Also applies to: 267-279, 294-306

---

470-472: Add documentation for can_undo?/1 function.

The can_undo?/1 function lacks documentation. Add a module doc explaining its purpose and return value.

+  @doc """
+  Checks if there are any operations that can be undone.
+
+  Returns `true` if there are operations in the undo stack, `false` otherwise.
+  """
   def can_undo?(manager) do
     Yex.Nif.undo_manager_can_undo(manager.reference)
   end

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: cortfritz
PR: satoren/y_ex#109
File: lib/undo_manager.ex:241-249
Timestamp: 2024-12-24T05:42:23.523Z
Learning: The function ensure_metadata_server will return {:ok, manager} when the server starts successfully or an error tuple otherwise, mitigating any race condition concerns.

#!/bin/bash
# Check for any remaining references to the old module name
rg "Yex\.UndoManager\.Options\." --type elixir

# Check for consistent usage of the new module name
rg "Yex\.UndoManager\." --type elixir

lib/undo_metadata_server.ex (3)

56-65: Add documentation for State struct fields.

The State struct would benefit from documentation explaining each field's purpose, particularly the metadata map structure and the role of next_id.

 defmodule State do
   @moduledoc false
+  @typedoc """
+  Internal state for the UndoMetadataServer.
+
+  Fields:
+  - item_added_callback: Callback function called when items are added
+  - item_updated_callback: Callback function called when items are updated
+  - item_popped_callback: Callback function called when items are popped
+  - metadata: Map of event_id to metadata for preserving data across operations
+  - next_id: Counter for generating unique event IDs
+  """
   defstruct item_added_callback: nil,
             item_updated_callback: nil,
             item_popped_callback: nil,
             metadata: %{},
             next_id: 0
 end

---

111-113: Add error handling for callback invocation.

The callback invocation should be wrapped in a try-catch block to handle potential errors gracefully.

 if state.item_added_callback do
-  callback_metadata = state.item_added_callback.(event)
+  callback_metadata = try do
+    state.item_added_callback.(event)
+  rescue
+    error ->
+      Logger.error("Error in item_added callback: #{inspect(error)}")
+      %{}
+  end

---

179-181: Add error handling for popped callback.

Similar to the item_added callback, wrap the callback invocation in a try-catch block.

 if state.item_popped_callback do
-  state.item_popped_callback.(id, event)
+  try do
+    state.item_popped_callback.(id, event)
+  rescue
+    error ->
+      Logger.error("Error in item_popped callback: #{inspect(error)}")
+  end
 end

native/yex/src/undo.rs (3)

30-44: Consider enhancing error handling for observer operations

While the observer management is well-implemented with proper cleanup in the Drop trait, consider adding error handling for observer operations to handle edge cases such as:

• Failed subscription creation
• Failed observer removal
• Concurrent modification of observers

 impl UndoManagerWrapper {
     pub fn new(manager: YrsUndoManager<UndoMetadata>) -> Self {
         Self {
             manager,
             item_added_observer: None,
             item_updated_observer: None,
             item_popped_observer: None,
         }
     }
+
+    // Helper to safely remove an observer
+    fn remove_observer(&mut self, observer: &mut Option<(LocalPid, Subscription)>) -> Result<(), String> {
+        if let Some((_pid, sub)) = observer.take() {
+            drop(sub);
+            Ok(())
+        } else {
+            Err("No observer to remove".to_string())
+        }
+    }
 }

Also applies to: 510-526

---

317-440: Refactor observer implementations to reduce duplication

The observer implementations share common patterns. Consider extracting shared functionality to reduce code duplication and improve maintainability.

+// Helper function to handle observer setup
+fn setup_observer<F>(
+    wrapper: &mut UndoManagerWrapper,
+    current_observer: &mut Option<(LocalPid, Subscription)>,
+    observer: LocalPid,
+    subscription_fn: F
+) -> NifResult<rustler::Atom>
+where
+    F: FnOnce() -> Subscription,
+{
+    // Remove existing observer if any
+    if let Some((_pid, sub)) = current_observer.take() {
+        drop(sub);
+    }
+
+    let subscription = subscription_fn();
+    *current_observer = Some((observer.clone(), subscription));
+    Ok(atoms::ok())
+}

 #[rustler::nif]
 pub fn undo_manager_observe_item_added(
     reference: ResourceArc<UndoManagerResource>,
     observer: LocalPid,
 ) -> NifResult<rustler::Atom> {
     let mut wrapper = reference
         .0
         .write()
         .map_err(|_| RustlerError::Term(Box::new("Failed to acquire write lock")))?;

-    if let Some((_pid, sub)) = wrapper.item_added_observer.take() {
-        drop(sub);
-    }
-
     let observer = observer.clone();
+    setup_observer(&mut wrapper, &mut wrapper.item_added_observer, observer, || {
         wrapper.manager.observe_item_added(move |_txn, event| {
             // ... rest of the implementation
         })
+    })
 }

---

283-315: Add documentation for type mapping logic

The type mapping functions would benefit from documentation explaining the mapping logic and potential edge cases.

+/// Maps Yrs event kinds to Elixir atoms
+/// This mapping is used to maintain consistency between Rust and Elixir representations
 fn map_event_kind(kind: yrs::undo::EventKind) -> Atom {
     match kind {
         yrs::undo::EventKind::Undo => atoms::undo(),
         yrs::undo::EventKind::Redo => atoms::redo(),
     }
 }

+/// Converts BranchPtr types to their string representations
+/// This function maps internal Yrs type names to more user-friendly string identifiers
+/// The mapping follows these rules:
+/// - YText -> "text"
+/// - YArray -> "array"
+/// - YMap -> "map"
+/// - YXmlText -> "xml_text"
+/// - YXmlElement -> "xml_element"
+/// - YXmlFragment -> "xml_fragment"
+/// Any unrecognized type is mapped to "unknown"
 fn map_parent_types(types: &[BranchPtr]) -> Vec<String> {
     types
         .iter()
         .map(|t| {

test/undo_manager_test.exs (3)

32-52: Enhance metadata server helper function robustness

The wait_for_metadata_server helper could benefit from improved error handling and documentation.

+@doc """
+Waits for the metadata server to start and become ready.
+
+## Parameters
+  - manager: The UndoManager instance
+  - timeout: Maximum time to wait in milliseconds (default: 2000)
+
+## Returns
+  - {:ok, manager} if the server starts successfully
+  - {:error, :timeout} if the server doesn't start within the timeout
+  - {:error, :server_crash} if the server crashes during startup
+"""
 defp wait_for_metadata_server(manager, timeout \\ 2000) do
   start_time = System.monotonic_time(:millisecond)
   wait_for_metadata_server(manager, timeout, start_time)
 end

 defp wait_for_metadata_server(manager, timeout, start_time) do
   cond do
-    # Check if we have a pid and it's alive
     is_pid(manager.metadata_server_pid) && Process.alive?(manager.metadata_server_pid) ->
       {:ok, manager}

-    # Check if we've exceeded our timeout
     System.monotonic_time(:millisecond) - start_time > timeout ->
       {:error, :timeout}

-    # If not, sleep briefly and try again
+    # Check if the server crashed
+    is_pid(manager.metadata_server_pid) && !Process.alive?(manager.metadata_server_pid) ->
+      {:error, :server_crash}
+
+    # Continue waiting
     true ->
       Process.sleep(10)
       wait_for_metadata_server(manager, timeout, start_time)
   end
 end

---

818-846: Consider splitting metadata persistence test into smaller focused tests

The metadata persistence test covers multiple aspects in a single test. Consider splitting it into smaller, focused tests for better maintainability and clarity.

-  test "demonstrates metadata persistence across operations" do
+  describe "metadata persistence" do
+    test "metadata is preserved when item is added", %{doc: doc, text: text} do
+      {:ok, manager} = UndoManager.new(doc, text)
+      test_pid = self()
+      test_metadata = %{custom: "data"}
+
+      {:ok, manager} =
+        UndoManager.on_item_added(manager, fn event ->
+          send(test_pid, {:added, event})
+          test_metadata
+        end)
+
+      Text.insert(text, 0, "test")
+      assert_receive {:added, added_event}
+      assert added_event.meta.event_id
+    end
+
+    test "metadata is preserved when item is popped", %{doc: doc, text: text} do
+      {:ok, manager} = UndoManager.new(doc, text)
+      test_pid = self()
+      test_metadata = %{custom: "data"}
+
+      {:ok, manager} =
+        UndoManager.on_item_popped(manager, fn id, event ->
+          send(test_pid, {:popped, id, event})
+        end)
+
+      Text.insert(text, 0, "test")
+      UndoManager.undo(manager)
+      assert_receive {:popped, _id, popped_event}
+      assert popped_event.meta.data.custom == test_metadata.custom
+    end
+  end

---

2012-2031: Enhance error handling test coverage

The metadata server failure test is good, but consider expanding it to cover more error scenarios systematically.

+  describe "error handling" do
+    test "handles metadata server startup failure", %{doc: doc, text: text} do
+      # Existing test implementation
+    end
+
+    test "handles metadata server crash after startup", %{doc: doc, text: text} do
+      {:ok, manager} = UndoManager.new(doc, text)
+      test_pid = self()
+
+      {:ok, manager} =
+        UndoManager.on_item_added(manager, fn event ->
+          send(test_pid, {:item_added, event})
+        end)
+
+      # Simulate server crash
+      Process.exit(manager.metadata_server_pid, :kill)
+
+      # Verify manager handles the crash gracefully
+      Text.insert(text, 0, "test")
+      refute_receive {:item_added, _}
+    end
+
+    test "handles concurrent metadata server operations", %{doc: doc, text: text} do
+      {:ok, manager} = UndoManager.new(doc, text)
+      test_pid = self()
+
+      # Test concurrent operations
+      tasks = for _ <- 1..10 do
+        Task.async(fn ->
+          UndoManager.on_item_added(manager, fn event ->
+            send(test_pid, {:item_added, event})
+          end)
+        end)
+      end
+
+      results = Task.await_many(tasks)
+      assert Enum.all?(results, &match?({:ok, _}, &1))
+    end
+  end

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: cortfritz
PR: satoren/y_ex#109
File: lib/undo_manager.ex:241-249
Timestamp: 2024-12-24T05:42:23.523Z
Learning: The function ensure_metadata_server will return {:ok, manager} when the server starts successfully or an error tuple otherwise, mitigating any race condition concerns.

 def clear(%__MODULE__{} = manager) do
   case Yex.Nif.undo_manager_clear(manager.reference) do
     {:ok, {}} ->
       if manager.metadata_server_pid do
-        :ok = Yex.UndoMetadataServer.clear_metadata(manager.metadata_server_pid)
+        case Yex.UndoMetadataServer.clear_metadata(manager.metadata_server_pid) do
+          :ok -> :ok
+          error -> error
+        end
+      else
+        :ok
       end
-
-      :ok

     error ->
       error
   end
 end

test/undo_manager_test.exs (3)

4-4: Consider adding more detailed logging or removing the Logger requirement if not used.
Requiring Logger here is fine, but it's unclear if you intend to log additional information during these tests. If not, you can consider removing it to reduce unused dependencies.

---

32-53: Recursive function for metadata server readiness is correct but consider exponential backoff.
This helper function correctly uses recursion, a timeout check, and a short sleep to wait for the metadata server to start. However, you may consider adding a backoff strategy or clearer logging to avoid potential repeated short sleeps in heavily loaded systems.

---

2012-2031: Metadata server startup failure is gracefully handled.
Mocking Yex.UndoMetadataServer.start_link/1 to force an error scenario thoroughly tests your fallback. You might log or expose a more descriptive error message to help with debugging, but the basic handling is solid.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/undo_manager.ex (1)

458-470: Enhance error handling in clear function.

The function should handle errors more explicitly and return consistent error tuples.

Apply this diff to improve error handling:

 def clear(%__MODULE__{} = manager) do
   case Yex.Nif.undo_manager_clear(manager.reference) do
-    {:ok, {}} ->
+    {:ok, {}} when is_nil(manager.metadata_server_pid) ->
+      :ok
+    {:ok, {}} ->
       if manager.metadata_server_pid do
-        :ok = Yex.UndoMetadataServer.clear_metadata(manager.metadata_server_pid)
+        case Yex.UndoMetadataServer.clear_metadata(manager.metadata_server_pid) do
+          :ok -> :ok
+          error -> {:error, {:metadata_clear_failed, error}}
+        end
       end
-
-      :ok
-
     error ->
-      error
+      {:error, {:manager_clear_failed, error}}
   end
 end

native/yex/src/undo.rs (2)

318-441: Consider enhancing error handling for observer message sending.

While the observer implementation is solid, the message sending could benefit from error handling.

Example improvement for the item_added observer (apply similar pattern to others):

-            let _ = env.send(&observer, message);
+            if let Err(e) = env.send(&observer, message) {
+                // Log error but don't fail - observers should not break functionality
+                eprintln!("Failed to send item_added message: {}", e);
+            }

---

293-316: Consider refactoring map_parent_types for better maintainability.

The current implementation could be more maintainable using a mapping structure.

Consider this approach:

 fn map_parent_types(types: &[BranchPtr]) -> Vec<String> {
+    const TYPE_MAPPINGS: &[(&str, &str)] = &[
+        ("YText", "text"),
+        ("YArray", "array"),
+        ("YMap", "map"),
+        ("YXmlText", "xml_text"),
+        ("YXmlElement", "xml_element"),
+        ("YXmlFragment", "xml_fragment"),
+    ];
+
     types
         .iter()
         .map(|t| {
             let type_str = t.to_string();
-            if type_str.starts_with("YText") {
-                "text".to_string()
-            } else if type_str.starts_with("YArray") {
-                "array".to_string()
-            } else if type_str.starts_with("YMap") {
-                "map".to_string()
-            } else if type_str.starts_with("YXmlText") {
-                "xml_text".to_string()
-            } else if type_str.starts_with("YXmlElement") {
-                "xml_element".to_string()
-            } else if type_str.starts_with("YXmlFragment") {
-                "xml_fragment".to_string()
-            } else {
-                "unknown".to_string()
-            }
+            TYPE_MAPPINGS
+                .iter()
+                .find(|(prefix, _)| type_str.starts_with(prefix))
+                .map(|(_, mapped)| mapped.to_string())
+                .unwrap_or_else(|| "unknown".to_string())
         })
         .collect()
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: cortfritz
PR: satoren/y_ex#109
File: lib/undo_manager.ex:241-249
Timestamp: 2024-12-24T05:42:23.523Z
Learning: The function ensure_metadata_server will return {:ok, manager} when the server starts successfully or an error tuple otherwise, mitigating any race condition concerns.

test/undo_manager_test.exs (6)

818-846: Add assertions for complete test coverage.

Consider adding these assertions to strengthen the test:

1. Verify the text content after insertion
2. Assert the complete metadata structure, not just the custom field

 test "demonstrates metadata persistence across operations", %{doc: doc, text: text} do
   {:ok, manager} = UndoManager.new(doc, text)
   test_pid = self()
   test_metadata = %{custom: "data"}

   # Register callbacks that use metadata
   {:ok, manager} =
     UndoManager.on_item_added(manager, fn event ->
       send(test_pid, {:added, event})
       test_metadata
     end)

   {:ok, manager} =
     UndoManager.on_item_popped(manager, fn id, event ->
       send(test_pid, {:popped, id, event})
     end)

   # Make changes
   Text.insert(text, 0, "test")
+  assert Text.to_string(text) == "test"

   # Verify added event
   assert_receive {:added, added_event}
   event_id = added_event.meta.event_id
+  assert is_struct(added_event.meta, UndoMetadata)

   # Undo and verify metadata persists
   UndoManager.undo(manager)
   assert_receive {:popped, ^event_id, popped_event}
   assert popped_event.meta.data.custom == test_metadata.custom
+  assert Text.to_string(text) == ""
 end

---

2012-2031: Enhance failure handling test coverage.

Consider adding these test cases to ensure robust failure handling:

1. Test recovery after failure (retry mechanism)
2. Test cleanup after failure
3. Verify system state remains consistent after failure

 test "handles metadata server startup failure", %{doc: doc, text: text} do
   {:ok, manager} = UndoManager.new(doc, text)
   test_pid = self()

   # Mock the metadata server to simulate startup failure
   with_mock Yex.UndoMetadataServer,
     start_link: fn _ref ->
       {:error, :test_startup_failure}
     end do
     # Attempt to register a callback which requires the metadata server
     result =
       UndoManager.on_item_added(manager, fn event ->
         send(test_pid, {:item_added, event})
       end)

     # Verify we get the error result
     assert result == {:error, :test_startup_failure}

     # Verify the manager's metadata_server_pid is still nil
     assert manager.metadata_server_pid == nil
+
+    # Verify system can still perform basic operations
+    Text.insert(text, 0, "test")
+    assert Text.to_string(text) == "test"
+
+    # Verify cleanup works properly after failure
+    assert :ok = UndoManager.cleanup(manager)
+
+    # Attempt recovery by creating a new manager
+    {:ok, new_manager} = UndoManager.new(doc, text)
+    assert is_struct(new_manager, UndoManager)
   end
 end

---

2149-2204: Add concurrent operation tests for cleanup.

The cleanup test should verify behavior during concurrent operations and active transactions:

 test "cleanup properly handles all cleanup tasks", %{doc: doc, text: text} do
   {:ok, manager} = UndoManager.new(doc, text)
   test_pid = self()

   # Set up observers
   {:ok, manager} =
     UndoManager.on_item_added(manager, fn event ->
       send(test_pid, {:item_added, event})
     end)

   {:ok, manager} =
     UndoManager.on_item_updated(manager, fn event ->
       send(test_pid, {:item_updated, event})
     end)

   {:ok, manager} =
     UndoManager.on_item_popped(manager, fn id, event ->
       send(test_pid, {:item_popped, id, event})
     end)

   # Wait for metadata server to be ready
   {:ok, manager} = wait_for_metadata_server(manager)
   metadata_server_pid = manager.metadata_server_pid

+  # Start a concurrent operation
+  task = Task.async(fn ->
+    for _ <- 1..100 do
+      Doc.transaction(doc, nil, fn ->
+        Text.insert(text, 0, "concurrent")
+        Process.sleep(10)
+        Text.delete(text, 0, 9)
+      end)
+    end
+  end)
+
   # Make some changes to create undo history
   Doc.transaction(doc, nil, fn ->
     Text.insert(text, 0, "Hello")
   end)

   UndoManager.stop_capturing(manager)

   # Verify we have undo history and an active metadata server
   assert UndoManager.can_undo?(manager)
   assert Process.alive?(metadata_server_pid)
   assert_receive {:item_added, _}

   # Call cleanup
   assert :ok = UndoManager.cleanup(manager)

+  # Wait for concurrent operation to finish
+  Task.await(task)
+
   # Verify:
   # 1. Undo stack is cleared
   refute UndoManager.can_undo?(manager)
   refute UndoManager.can_redo?(manager)

   # 2. Metadata server is stopped
   refute Process.alive?(metadata_server_pid)

   # 3. Observers are removed (make a change and verify no callbacks)
   Doc.transaction(doc, nil, fn ->
     Text.insert(text, 0, "Test")
   end)

   refute_receive {:item_added, _}
   refute_receive {:item_updated, _}
   refute_receive {:item_popped, _, _}
+
+  # 4. Verify no memory leaks
+  assert Process.info(self(), :memory) < 1_000_000
 end

---

1570-1795: Reduce test duplication using shared helpers.

The origin tracking tests have significant duplication. Consider refactoring using shared helpers:

+defp test_origin_tracking(doc, type, type_module, insert_fn, to_string_fn) do
+  {:ok, manager} = UndoManager.new(doc, type)
+  origin = "test-origin"
+  UndoManager.include_origin(manager, origin)
+
+  # Make tracked changes
+  Doc.transaction(doc, origin, fn ->
+    insert_fn.(type, "tracked")
+  end)
+
+  # Make untracked changes
+  Doc.transaction(doc, "other-origin", fn ->
+    insert_fn.(type, "untracked")
+  end)
+
+  # Undo should only remove tracked changes
+  UndoManager.undo(manager)
+
+  # Verify only untracked changes remain
+  assert to_string_fn.(type) =~ "untracked"
+  refute to_string_fn.(type) =~ "tracked"
+end
+
 test "origin tracking works with text type", %{doc: doc, text: text} do
-  {:ok, manager} = UndoManager.new(doc, text)
-  origin = "test-origin"
-  UndoManager.include_origin(manager, origin)
-
-  # Make tracked changes
-  Doc.transaction(doc, origin, fn ->
-    Text.insert(text, 0, "tracked")
-  end)
-
-  # Make untracked changes
-  Doc.transaction(doc, "other-origin", fn ->
-    Text.insert(text, 0, "tracked")
-  end)
-
-  # Undo should only remove tracked changes
-  UndoManager.undo(manager)
-
-  # Verify only untracked changes remain
-  assert Text.to_string(text) == "tracked"
+  test_origin_tracking(
+    doc,
+    text,
+    Text,
+    &Text.insert(&1, 0, &2),
+    &Text.to_string/1
+  )
 end

---

1177-1406: Add edge case tests for observer callbacks.

Consider adding these test cases to ensure robust callback handling:

+test "handles observer callback errors gracefully", %{doc: doc, text: text} do
+  {:ok, manager} = UndoManager.new(doc, text)
+  test_pid = self()
+
+  # Register callback that raises an error
+  {:ok, manager} =
+    UndoManager.on_item_added(manager, fn event ->
+      send(test_pid, {:before_error, event})
+      raise "Simulated callback error"
+    end)
+
+  # Make changes - should not crash
+  Text.insert(text, 0, "test")
+
+  # Verify the error was handled
+  assert_receive {:before_error, _}
+  assert Text.to_string(text) == "test"
+end
+
+test "handles observer callback timeouts", %{doc: doc, text: text} do
+  {:ok, manager} = UndoManager.new(doc, text)
+  test_pid = self()
+
+  # Register callback that times out
+  {:ok, manager} =
+    UndoManager.on_item_added(manager, fn event ->
+      send(test_pid, {:before_sleep, event})
+      Process.sleep(5000)  # Longer than default timeout
+      send(test_pid, :after_sleep)
+    end)
+
+  # Make changes - should not hang
+  Text.insert(text, 0, "test")
+
+  # Verify the timeout was handled
+  assert_receive {:before_sleep, _}
+  refute_receive :after_sleep
+  assert Text.to_string(text) == "test"
+end

---

1797-1805: Enhance supervisor specification testing.

The child_spec test should verify all supervisor-related configurations:

 test "child_spec returns valid supervisor child specification", %{doc: doc, text: text} do
   child_spec = UndoManager.child_spec({doc, text})

-  assert child_spec == %{
-            id: Yex.UndoManager,
-            start: {Yex.UndoManager, :new, [doc, text]},
-            type: :worker
-          }
+  # Verify all supervisor-related configurations
+  assert %{
+    id: Yex.UndoManager,
+    start: {Yex.UndoManager, :new, [doc, text]},
+    type: :worker,
+    restart: :permanent,  # Verify restart strategy
+    shutdown: 5000       # Verify shutdown timeout
+  } = child_spec
+
+  # Verify the child can be started by a supervisor
+  {:ok, sup_pid} = Supervisor.start_link([child_spec], strategy: :one_for_one)
+  assert [{:undefined, child_pid, :worker, [Yex.UndoManager]}] = Supervisor.which_children(sup_pid)
+  assert Process.alive?(child_pid)
+
+  # Cleanup
+  Supervisor.stop(sup_pid)
 end

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/undo_manager.ex (3)

4-88: Enhance error scenarios documentation.

The module documentation thoroughly explains the functionality but could benefit from a dedicated section about error handling and recovery strategies. Consider adding examples of:

• How to handle metadata server failures
• Recovery strategies for observer callback errors
• Best practices for error handling in callbacks

---

198-222: Consider adding exponential backoff to retry mechanism.

The retry mechanism is a good addition, but it could be improved by implementing exponential backoff to handle transient failures more gracefully.

 defp try_create_manager(doc, scope, options, attempts) do
+  # Add delay between retries with exponential backoff
+  if attempts < 3, do: Process.sleep(100 * (3 - attempts))
   case Yex.Nif.undo_manager_new_with_options(doc, scope, options) do
     {:ok, rust_ref} ->

---

459-471: Improve error pattern matching in clear function.

The function could benefit from more specific error pattern matching to handle different failure scenarios appropriately.

 def clear(%__MODULE__{} = manager) do
   case Yex.Nif.undo_manager_clear(manager.reference) do
     {:ok, {}} ->
       if manager.metadata_server_pid do
-        :ok = Yex.UndoMetadataServer.clear_metadata(manager.metadata_server_pid)
+        case Yex.UndoMetadataServer.clear_metadata(manager.metadata_server_pid) do
+          :ok -> :ok
+          {:error, reason} = error ->
+            Logger.warning("Failed to clear metadata: #{inspect(reason)}")
+            error
+        end
       end

-      :ok
-
-    error ->
-      error
+    {:error, :invalid_reference} ->
+      {:error, :invalid_manager}
+    {:error, reason} = error ->
+      Logger.error("Failed to clear undo manager: #{inspect(reason)}")
+      error
   end
 end

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: cortfritz
PR: satoren/y_ex#109
File: lib/undo_manager.ex:241-249
Timestamp: 2024-12-24T05:42:23.523Z
Learning: The function ensure_metadata_server will return {:ok, manager} when the server starts successfully or an error tuple otherwise, mitigating any race condition concerns.

+  @doc false
+  def terminate(_reason, %__MODULE__{} = manager) do
+    cleanup(manager)
+  end

   def cleanup(%__MODULE__{} = manager) do

test/undo_manager_test.exs (1)

926-942: 🛠️ Refactor suggestion

Replace commented sleeps with proper async handling.

The commented Process.sleep calls suggest potential race conditions. Consider implementing proper async handling:

1. Use ExUnit.Callbacks.start_supervised/2 for process management
2. Implement proper wait mechanisms or use message passing for synchronization

 # Add some XML content
 Doc.transaction(doc, nil, fn ->
   XmlFragment.push(xml_fragment, XmlTextPrelim.from("Hello"))
   XmlFragment.push(xml_fragment, XmlElementPrelim.empty("div"))
 end)

-# Give time for the transaction to complete
-# Process.sleep(10)
+# Wait for transaction completion
+assert_receive {:transaction_complete, _}, 1000

 # Verify initial state
 assert XmlFragment.to_string(xml_fragment) == "Hello<div></div>"

 # Undo changes
 UndoManager.undo(undo_manager)

-# Give time for the undo operation to complete
-# Process.sleep(10)
+# Wait for undo completion
+assert_receive {:undo_complete, _}, 1000

test/undo_manager_test.exs (1)

32-52: Add documentation and type specs to helper functions.

The helper functions would benefit from:

1. Module documentation explaining their purpose
2. Type specs for better type checking and documentation

+@doc """
+Waits for the metadata server to start up and become ready.
+
+## Parameters
+  - manager: The UndoManager struct
+  - timeout: Maximum time to wait in milliseconds (default: 2000)
+
+## Returns
+  - {:ok, manager} if the server is ready
+  - {:error, :timeout} if the timeout is exceeded
+"""
+@spec wait_for_metadata_server(UndoManager.t(), non_neg_integer()) ::
+        {:ok, UndoManager.t()} | {:error, :timeout}
 defp wait_for_metadata_server(manager, timeout \\ 2000) do
   start_time = System.monotonic_time(:millisecond)
   wait_for_metadata_server(manager, timeout, start_time)
 end

+@doc false
+@spec wait_for_metadata_server(UndoManager.t(), non_neg_integer(), non_neg_integer()) ::
+        {:ok, UndoManager.t()} | {:error, :timeout}
 defp wait_for_metadata_server(manager, timeout, start_time) do

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

bench/undo_manager_stress.exs (4)

3-7: Consider making test parameters configurable
You’ve defined @doc_size, @num_actors, and @test_duration_ms as module attributes. This is convenient, but each environment may require different stress levels. Consider making these parameters overridable at runtime (e.g., via command-line args or environment variables) to allow more flexible testing.

---

64-65: Leverage function naming to clarify scope
actor_loop/7 does a significant amount of random operation handling and concurrency. Consider splitting it into smaller functions for each operation type and a separate orchestration function. This makes testing and maintenance easier.

---

66-112: Add robust error handling or retry logic
If random operations fail, you rescue and report them. This is good for test coverage, but if some errors are transient, you could introduce optional retry or ensure that repeated exceptions are properly logged.

---

157-172: Consider saving metrics results to disk or external storage
Currently, metrics are only printed. For deeper analysis of stress test results over time, you might store them in a file or push them to a more robust data sink.

bench/observer_stress.exs (4)

3-8: Use a Supervisor to manage long-lived processes
You spawn multiple processes for concurrency, including progress indicators and actor creation. Consider a Supervisor-based approach (or use a task supervision strategy) to manage these processes. This would help ensure cleanup on failure and lifecycle management.

---

72-82: Ensure observer callbacks do meaningful work
Here, the callbacks simply return a map with %{actor_id: id, type: :updated} or similar. If these callbacks are placeholders, ensure the real logic either uses the callback data or logs it for debugging. Otherwise, consider limiting them for clarity.

---

137-163: Avoid potential memory bloat in MemoryMetrics
Each call to record_memory_point/1 inserts a record into :memory_metrics. For longer runs, the ETS table could grow large. You might prune old records or sample memory usage periodically.

---

200-212: Provide deeper memory usage analysis
Printing only min/max/avg memory is helpful, but for large-scale concurrency, consider more advanced percentile metrics (e.g., 95th percentile) or time-series plots for more detailed analysis.

bench/doc_stress.exs (3)

9-16: Parametrize initial content size
You insert a 200,000-character string at line 18. Similar to other stress tests, consider making this value configurable for different test environments or for quick experiments.

---

79-103: Guard operations for potential edge cases
The text, map, array, and xml insertions happen randomly. If random positions or keys exceed the intended range or cause duplicates, ensure the stress test continues gracefully without halting.

---

158-161: Consider referencing OS-level memory stats
Beyond the built-in :erlang.memory, you could gather OS-level metrics (if available) to measure total system usage, swap, etc., especially when the code runs on large servers.

Would you like a follow-up script that attempts to gather cgroup or /proc/meminfo data for a more holistic memory analysis?

bench/undo_manager_bench.exs (2)

3-9: Consider additional random data or parallel runs
The current setup function inserts “Hello” or “Hello World” in deterministic positions. For more realistic performance benchmarks, consider randomizing the content or performing insertions concurrently.

---

39-46: Use more complex observer callbacks in benchmarking
The “observer callbacks” test only inserts a small piece of text. Realistic usage might include multiple changes in quick succession to measure overhead.

test/undo_manager_test.exs (3)

103-105: Handle exceptions gracefully in concurrency-based tests
Rescuing a generic error is fine for quick debugging, but if unexpected errors occur, you might lose context about the transaction state. Consider structured logging or specialized error handling.

---

631-633: Duck-type check for references carefully
You assert is_reference(undo_manager.reference). This is fine, but consider also verifying that the reference points to a valid internal structure or handle if the NIF or GenServer fails to initialize.

---

1596-1602: Time-consuming callbacks should be handled with caution
The callback that sleeps 5s demonstrates a potential stall. If real callbacks might run that long, consider a queue or background worker approach to avoid potential starvations or open many processes.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro